Skip to content

fix buffer full deadlock#2929

Merged
Adam-D-Lewis merged 4 commits intomainfrom
fix_full_buffer
Feb 20, 2025
Merged

fix buffer full deadlock#2929
Adam-D-Lewis merged 4 commits intomainfrom
fix_full_buffer

Conversation

@Adam-D-Lewis
Copy link
Copy Markdown
Member

@Adam-D-Lewis Adam-D-Lewis commented Jan 29, 2025 •
edited
Loading

Reference Issues or PRs

Fixes #2928

What does this implement/fix?

See #2928

Put a x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

How to test this PR?

git checkout 0fff8bb # adds test which shows issue
pytest tests/tests_unit/test_utils.py -k test_run_subprocess_cmd # fails due to timeout from deadlock issue

git checkout fix_full_buffer # fixes issue
pytest tests/tests_unit/test_utils.py -k test_run_subprocess_cmd # passes

Any other comments?

@Adam-D-Lewis Adam-D-Lewis linked an issue Jan 29, 2025 that may be closed by this pull request
[BUG] - Pipe buffer deadlock #2928
Closed
@Adam-D-Lewis
Copy link
Copy Markdown
Member Author

Adam-D-Lewis commented Jan 29, 2025 •
edited
Loading

This PR may still not properly handle color codes.

image

Update: I believe this is a separate issue.

@Adam-D-Lewis
Copy link
Copy Markdown
Member Author

Adam-D-Lewis commented Jan 29, 2025 •
edited
Loading

Also, seems like some file is not being closed correctly when looking at the error in the tests.

Update: fixed

@dcmcand
Copy link
Copy Markdown
Contributor

dcmcand commented Feb 7, 2025

@Adam-D-Lewis can you fill out the how to test section?

@dcmcand dcmcand requested a review from a team as a code owner February 7, 2025 14:17
@Adam-D-Lewis
Copy link
Copy Markdown
Member Author

Adam-D-Lewis commented Feb 10, 2025

@Adam-D-Lewis can you fill out the how to test section?

Now I added that section (plus a test)

@dcmcand dcmcand added the status: approved 💪🏾 This PR has been reviewed and approved for merge label Feb 20, 2025
@Adam-D-Lewis Adam-D-Lewis merged commit 13af52c into main Feb 20, 2025
26 checks passed
@Adam-D-Lewis Adam-D-Lewis deleted the fix_full_buffer branch February 20, 2025 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dcmcand dcmcand dcmcand approved these changes

@viniciusdc viniciusdc Awaiting requested review from viniciusdc

Assignees

No one assigned

Labels

status: approved 💪🏾 This PR has been reviewed and approved for merge

Projects

🪴 Nebari Project Management
Status: Done 💪🏾

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] - Pipe buffer deadlock

2 participants

@Adam-D-Lewis @dcmcand